Do we actually need this? I'm pretty sure the old slice goes out of scope at the end of this function and the *pq = old[0:n-1] is copying its (ptr, len, cap)`, not its contents. https://go.dev/play/p/xE0glBdR8O6

Maybe I'm missing something?

Admittedly I took this directly from the container/heap priorityQueue example here.

I'll take a deeper look at this today.

I did some quick digging and found golang/go#65403 and golang/go#65404 which leads to this Gerrit discussion https://go-review.googlesource.com/c/go/+/559775

I guess I'm wrong?

https://go.dev/play/p/7hR-IWTT7J9 shows it: the old pointer still lives in the backing array. If you uncomment the old[n-1] = nil and re-run this, it'll show as 0x0 (nil)

Yeah I was wondering if this has to do with the fact the capacity of the slice is still the same, so it's probably still holding onto that pointer, even though it doesn't appear so?

I realize this isn't wired-up yet but do we imagine we'll return the empty workload here if this is a non-batch job, or will we just not call Enqueue for those in the first place?

I haven't completely figured out what the best way to "route" these evaluation is yet. I was thinking that only batch jobs would be routed to this Queue, and then if for example, they didn't have the required metadata flag (if metadata was set), then we would just pass to the eval broker?

Open to ideas here though.

Suppose a job author writes a job that can't ever be placed because they screwed up a constraint. Doesn't this end up blocking forever and preventing any further jobs from being enqueued to the eval broker? Do we need some way of "abandoning" a workload in this queue or otherwise saving it to be retried later?

Also, we don't do anything with the error returned from this.

Yeah at the moment this would block forever until the job was stopped and eval was marked complete. We could add some configurable limit to waitForPlacements that stops the blocking query after some period of time has gone by. I'm not sure there much we can do in the way of saving it to be retried later, as once it's released to the eval broker, it's now out of our hands.

Yeah I forgot to handle this error, I'll get that updated. 😄

I'm not sure there much we can do in the way of saving it to be retried later, as once it's released to the eval broker, it's now out of our hands.

That's a good point, but also makes me realize a more fundamental issue: won't any blocked eval also end up being re-submitted to this queue? Which means we'll be waiting here and never enqueing the blocked eval into the eval broker in the first place. Right now I don't think we ever unblock in the case of a blocked eval. We should probably add a test that covers this workflow.

We would filter out any evals that are not Eval.TrriggeredBy == EvalTriggerJobRegister, so we should be good there. But it will get a little complex with new versions of jobs, but I'm just trying to get a basic queue in here.

Hopefully next I'll start wiring it up, and working on state restore, which also will be a little complex because of leadership transfers.

Definitely a TBD, but do we think we're just going to blow away the whole queue if this configuration gets changed via API?

Yeah I think just getting a solid "restore the queue from state" functionality, and relying on it for any conf changes is probably the best way to go at least for now.

Strictly speaking this doesn't reliably exercise the desired behavior: waitForPlacement could still be on its first pass through the loop and not have had an opportunity to incorrectly return. If waitForPlacement were buggy this would be flaky rather than always fail.

I think the next subtest has the same problem?

Yeah you're absolutely right, I'll look into a better way to write this test.

Added a wait for the new test watchset which guarentees that the goroutine has begun it's actual blocking query before we upsert an eval update.